Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colorize weblist output #271

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Colorize weblist output #271

wants to merge 8 commits into from

Conversation

Maaarcocr
Copy link

@Maaarcocr Maaarcocr commented Nov 29, 2017

This is my first try at solving #218 (and also first time contributing here 😄)

This is how it looks:
image

I have added 5 new CSS classes whose background-colors are from a red to yellow gradient (please feel free to criticize my color choices)
I am not sure if I should use flatSum or cumSum in order to calculate the percentile, so if you have any opinion on this, please let me know.

I can add some basics tests for the calculatePercentile function, if needed.

@Maaarcocr
Copy link
Author

As you may have noticed I have some failing tests because of the different html output for weblist. Is there a way of updating the testdata easily?

<span class=line> 5</span> <span class=nop> . . line5 </span>
<span class=line> 6</span> <span class=nop> . . line6 </span>
<span class=line> 7</span> <span class=nop> . . line7 </span>
<span class=line> 3</span> <span class="nop "> . . line3 </span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing space shouldn't be there.

@@ -347,20 +347,40 @@ func printFunctionHeader(w io.Writer, name, path string, flatSum, cumSum int64,
measurement.Percentage(cumSum, rpt.total))
}

func calculatePercentile(partial, sum int64) string {
percentile := partial * 100 / sum
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not percentile.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change this. Misunderstood its meaning.

Copy link
Author

@Maaarcocr Maaarcocr Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aalexand I have now implemented percentile. I'm not sure if my code (in terms of style and correctness) is fine, thus I have pushed a first version without tests, to get it reviewed. I will add tests soon.

I wonder if the percentiles functions should go into a different file or if they belong where they are.

@codecov-io
Copy link

codecov-io commented Dec 1, 2017

Codecov Report

Merging #271 (3e3a1c0) into master (3ec35eb) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   67.14%   67.25%   +0.10%     
==========================================
  Files          78       78              
  Lines       14084    14130      +46     
==========================================
+ Hits         9457     9503      +46     
  Misses       3792     3792              
  Partials      835      835              
Impacted Files Coverage Δ
internal/report/source_html.go 0.00% <ø> (ø)
internal/report/source.go 82.68% <100.00%> (+1.27%) ⬆️
.../github.com/google/pprof/internal/report/source.go 82.68% <0.00%> (+1.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ec35eb...3e3a1c0. Read the comment docs.

@aalexand
Copy link
Collaborator

aalexand commented Dec 2, 2017

Looks like test fail at formatting check.

@Maaarcocr
Copy link
Author

@aalexand solved 😄 I was sleeping so I couldn't fix it straight away.

Copy link
Collaborator

@aalexand aalexand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of style comments but also one important one on the what we actually want to do, please look at that first (it's the longest one).

}
printFunctionClosing(w)
}
return nil
}

func getPercentileString(cumSum float64, percentiles map[float64]float64) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function can be simplified with a for loop, there is unnecessary repetition.

for _, fn := range fnodes {
printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, rpt)
printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, getPercentileString(float64(fn.Cum), percentiles), rpt)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getPercentileString - this is not just string, this is CSS class name?

}
printFunctionClosing(w)
}
return nil
}

func getPercentileString(cumSum float64, percentiles map[float64]float64) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using float64 as the map key is quite unusual.

}
printFunctionClosing(w)
}
return nil
}

func getPercentileString(cumSum float64, percentiles map[float64]float64) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest replacing "Percentile" with "Ptile" throughout. Using the full term makes the code unnecessarily verbose for the task.

return ""
}

// calculate percentile expects cumSums to be sorted and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc should start with function name.

"github.com/google/pprof/profile"
)

func TestCalculatePercentiles(t *testing.T) {
for _, testCase := range []struct {
fnodes graph.Nodes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "f" in "fnodes" stand for here?

func TestCalculatePercentiles(t *testing.T) {
for _, testCase := range []struct {
fnodes graph.Nodes
output map[float64]float64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

output -> want

return ""
}
switch {
case cumSum >= percentiles[80]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose the specific percentiles of 10, 20, 40, 60 and 80?

Looking at the whole PR, I think that we should start with something simpler.

  • Let's start with just two colors: pink and red.
  • Percentiles which are calculated off unique set built of float64 numbers look weird, frankly, so not sure it was a good idea. I think the easier to understand thing is to base the coloring on the max value among of the source lines.
    • Color as pink values that are > 80% of the max value.
    • Color as red values that are > 95% of the max value.
    • Leave all other values as white.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented these changes. The code now calculates the percentile on the array of cum values, instead of a set. I'm not using float64s anymore, instead I use int64. This last change meant that I had to implement the Sort Interface for int64.

return cumSums[int64(rank)]
}

func getSetOfCumValues(fnodes graph.Nodes) []float64 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should explain what this function is doing and why. E.g. the choice of deduplicating the different values before computing the percentile values is non-obvious, we should clearly explain the tactics and document the rationale.

"github.com/google/pprof/profile"
)

func TestCalculatePercentiles(t *testing.T) {
for _, testCase := range []struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be at least these additional test cases:

  • Duplicate values.
  • All equal values.

@aalexand
Copy link
Collaborator

@Maaarcocr Could you address the PR comments?

@Maaarcocr
Copy link
Author

@aalexand Sorry for the delay. I've been ill and I had a lot of coursework in the meanwhile, I will address the comments either today or tomorrow.

@Maaarcocr Maaarcocr closed this Dec 18, 2017
@Maaarcocr Maaarcocr reopened this Dec 18, 2017
@Maaarcocr
Copy link
Author

@aalexand I've changed the code based on your suggested changes.

I was wondering if I should change the background-color in a different way, such as:
In getPtileCSSClassName I could instead produce a style string and we wouldnt not use classes anymore. This way, if we want to add more percentiles we would not need to manually change the HTML.

@Maaarcocr
Copy link
Author

Hi! I would love to get this merged if it is still something that the project need 😄

@aalexand aalexand requested a review from rauls5382 May 30, 2018 18:27
@zchee
Copy link
Contributor

zchee commented Mar 18, 2021

@aalexand any progress this PR...? would be great feature I think.

@aalexand
Copy link
Collaborator

@zchee the branch is out of date so the author will need to rebase it first.

@zchee
Copy link
Contributor

zchee commented Mar 18, 2021

@aalexand Ah, your correct.

@Maaarcocr could you rebase it?

@Maaarcocr
Copy link
Author

Sorry for the very delayed answer. Will try to rebase when I have some free time, hopefully this weekend!

@zdyj3170101136
Copy link

still not merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants